fix units in channels.tsv for EEG (& other Volt-based) channels #1477
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1477 +/- ##
==========================================
- Coverage 96.94% 96.91% -0.03%
==========================================
Files 43 43
Lines 10077 10080 +3
==========================================
Hits 9769 9769
- Misses 308 311 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| # https://github.com/bids-standard/pybv/blob/2832c80ee00d12990a8c79f12c843c0d4ddc825b/pybv/io.py#L40 | ||
| # https://github.com/mne-tools/mne-bids/blob/1e0a96e132fc904ba856d42beaa9ddddb985f1ed/mne_bids/write.py#L1279-L1280 | ||
| if convert_fmt: | ||
| volt_like = "µV" if convert_fmt in ("BrainVision", "EDF", "EEGLAB") else "V" |
There was a problem hiding this comment.
this logic implies that all file formats except bv, edf and eeglab store voltages in V.
is this correct?
or is it rather correct, that all other formats dont do any conversion? then we would still need to access orig_unit here
maybe practically irrelevant if mne_bids only supports these export formats (for eeg they are currently the only formats allowed in BIDS), but couldnt there be MEG formats that store both MEG and EEG in the same file (like curry does)?
There was a problem hiding this comment.
MNE-BIDS doesn't allow exporting eeg, ieeg, seeg, etc to anything other than these 3 formats when eeg, etc are the primary datatype. If there are EEG channels in a file that also contains MEG channels, they should remain as V (unprocessed MEG data must remain in the native format of the instrument; AFAIK no MEG instruments natively save as EDF, .set, or BV). So given that these 3 are the only permitted formats for data where EEG/iEEG/sEEG is the primary datatype, I think the logic is correct.
There was a problem hiding this comment.
ok, I wasnt sure about the BIDS rules for MEG data.
if format conversion is not allowed for combined MEG/EEG files, there shouldn't be a problem, as the code will always access orig_unit.
e.g. in the curry MEG files i saw, original units of recording are fT and µV. if they are not converted, this will be written to channels.tsv
likewise, if ppl kick out all MEG channels to store EEG as a separate modality, they have to pick either of the 3 specified formats.
but if i follow correctly, the else in your line 211 must never be reached then?
i.e. convert_fmt and convert_fmt not in ("BrainVision", "EDF", "EEGLAB") is not allowed.
wouldnt it then be better to write smth like
if convert_fmt:
assert convert_fmt in ("BrainVision", "EDF", "EEGLAB")
volt_like = "µV"
or just
if convert_fmt and convert_fmt in ("BrainVision", "EDF", "EEGLAB"):
volt_like = "µV"
There was a problem hiding this comment.
If user reads MEG data in Curry format and is outputting FIFF then convert_fmt="FIFF" and the else clause is reached, no?
There was a problem hiding this comment.
unprocessed MEG data must remain in the native format of the instrument
i though you said that this is not allowed?
anyway, I'll stop questioning :)
if we are positive that all possible conversion output formats except bv, edf, and eeglab always store V (regardless of original unit), the logic is fine
There was a problem hiding this comment.
Ah yes, you're correct. but there are exceptions, e.g. if anonymization is needed then conversion may happen anyway
There was a problem hiding this comment.
Also the user can request/force conversion in spite of what the bids standard requires
3 participants
PR Description
All valid EEG export formats (BrainVision, EDF, and EEGLAB .set) convert V -> µV when writing, but µV wasn't getting written to channels.tsv units column in many cases.
see #1475 (comment)
Merge checklist
Maintainer, please confirm the following before merging.
If applicable: